refactor(api): requireAuth returns resolved Principal (closes #194)#864
refactor(api): requireAuth returns resolved Principal (closes #194)#864cristim wants to merge 3 commits into
Conversation
authenticatePrincipal resolves the caller's identity once across all three credential kinds (admin API key, user API key, bearer session) and returns a *Principal. requireAuth now delegates to it instead of calling authenticate() which threw the resolved identity away. Router updated to _, err := r.h.requireAuth(...). Nil-auth guard moved before the API-key path so a missing auth service fails closed (401) rather than falling through. Dead userIface/userFields locals removed. TestRequireAuth_* tests extended to assert the returned Principal (Kind, Role, UserID, Session pointer) for each credential type.
|
Warning Review limit reached
More reviews will be available in 72 minutes and 9 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Extract principalFromUserAPIKey and principalFromBearerToken helpers from authenticatePrincipal to bring its cyclomatic complexity from 11 down to 4, passing the gocyclo <=10 gate.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
Rate Limit Exceeded
|
…I-key and nil-auth paths (refs #194) When ValidateUserAPIKeyAPI returns a userRaw value whose concrete type does not satisfy the GetID/GetEmail/GetRole interface, principalFromUserAPIKey previously returned a partially-populated Principal (Role:"user", empty UserID/Email). Any handler trusting the Principal would have granted access to an unresolved identity. Fix: return nil (deny) when the assertion fails. Adds three tests: - TestRequireAuth_UserAPIKey: valid user API key yields PrincipalUserAPIKey with populated UserID/Email/Role (zero coverage on the #178 hot-spot). - TestRequireAuth_UserAPIKey_BadRecord: unexpected userRaw type yields 401; fails pre-fix (Principal returned), passes post-fix (nil returned). - TestRequireAuth_NilAuth_Rejects: Handler with auth==nil returns 401 for a non-admin key, confirming the fail-closed nil-auth branch.
|
@coderabbitai review |
✅ Action performedReview finished.
|
Summary
requireAuthnow returns(*Principal, error)instead of bareerrorby delegating to the newauthenticatePrincipalhelper (already added in this branch)authenticatePrincipalcleaned up: nil-auth guard moved before the API-key path (fail-closed perfeedback_fail_closed_middleware.md), deaduserIface/userFieldslocals removed_, err := r.h.requireAuth(...)to handle the new return shapeTestRequireAuth_*tests upgraded to assert the returnedPrincipal(kind, role, UserID, Session pointer) for each credential typeTest plan
go build ./...cleango test github.com/LeanerCloud/CUDly/internal/api/... github.com/LeanerCloud/CUDly/internal/auth/...— 1852 passedTestRequireAuth_AdminAPIKeynow assertsPrincipalAdminAPIKey/role=adminTestRequireAuth_UserSessionnow assertsPrincipalSession,UserID,Role,SessionpointerTestRequireAuth_NoCredential_Rejectsstill asserts 401 ClientErrorCloses #194